Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: at_commons changes for setting isEncrypted for both true and false #669

Merged
merged 6 commits into from
Sep 27, 2024
Merged

feat: at_commons changes for setting isEncrypted for both true and false #669

merged 6 commits into from
Sep 27, 2024

Conversation

murali-shris
Copy link
Member

- What I did

  • changes for setting isEncrypted in metadata for update verb for both true and false. Previously isEncrypted is set only when true
    - How I did it
  • modified atKey.metadata.toProtocolFragment
  • removed adhoc code for notify
  • modified test assertions
    - How to verify it
  • tests should pass

@murali-shris murali-shris mentioned this pull request Sep 25, 2024
Merged
gkc
gkc requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@gkc gkc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Given that isEncrypted defaults to false today
  • Then an app built with an older version of at_client but this version of at_commons will start emitting 'isEncrypted:false' in the metadata even though the data will actually be encrypted
  • Can you think through the ramifications of this please, @murali-shris - do we need an at_commons major version change?

@murali-shris
Copy link
Member Author

  • Given that isEncrypted defaults to false today
  • Then an app built with an older version of at_client but this version of at_commons will start emitting 'isEncrypted:false' in the metadata even though the data will actually be encrypted
  • Can you think through the ramifications of this please, @murali-shris - do we need an at_commons major version change?

Right Gary. Older version of atClient with latest at_commons version will have encrypted data with isEncrypted false.
Impact of this will be similar to existing data for shared/self keys. There is no functional breakage but will end up with more incorrect metadata(data is encrypted but isEncrypted =false)
I guess it is better to have a major release for this change.

@murali-shris murali-shris requested a review from gkc September 26, 2024 10:49
gkc
gkc reviewed Sep 27, 2024
View reviewed changes
packages/at_commons/CHANGELOG.md Outdated
@@ -1,4 +1,5 @@
## 4.1.3
## 5.0.0
- [Breaking Change]feat: Add isEncrypted flag to update verb metadata for both true and false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to "Emit the isEncrypted value in the metadata if it is false"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@murali-shris murali-shris requested a review from gkc September 27, 2024 08:34
@gkc gkc merged commit af5f03b into trunk Sep 27, 2024
11 checks passed
@gkc gkc deleted the update_isencrypted_changes branch September 27, 2024 09:08
@murali-shris murali-shris mentioned this pull request Sep 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkc gkc gkc approved these changes

@sitaram-kalluri sitaram-kalluri Awaiting requested review from sitaram-kalluri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@murali-shris @gkc